ENH/FIX: Resolve LinAlgError during SVD #2838
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The default `lapack_driver` uses randomized SVD, which occasionally will cause `LinAlgError` even if the data are completely fine. Switching `lapack_driver` to `gesvd` will solve this issue.
effigies
reviewed
Dec 21, 2018
nipype/algorithms/confounds.py
Outdated
| @@ -1193,9 +1194,12 @@ def compute_noise_components(imgseries, mask_images, num_components, | |||
| try: | |||
| u, _, _ = np.linalg.svd(M, full_matrices=False) | |||
There was a problem hiding this comment.
Nested try/except blocks get a little hard to reason about. How about a helper function that will do the fall-back:
# Use the numpy.linalg.svd protocol
def fallback_svd(a, full_matrices=True, compute_uv=True):
try:
return np.linalg.svd(a, full_matrices=full_matrices, compute_uv=compute_uv)
except np.linalg.LinAlgError:
pass
from scipy.linalg import svd
return svd(a, full_matrices=full_matrices, compute_uv=compute_uv, lapack_driver='gesvd')And then we just replace the above with:
Suggested change
| u, _, _ = np.linalg.svd(M, full_matrices=False) | |
| u, _, _ = fallback_svd(M, full_matrices=False) |
There was a problem hiding this comment.
And to be clear, I'm suggesting we place the above fallback_svd function at the file level.
There was a problem hiding this comment.
Great idea! By putting it at the file level do you mean putting it at the top of the file?
There was a problem hiding this comment.
Yes. Anywhere, really, but not nested within a class or another function. The top is fine.
Codecov Report
@@ Coverage Diff @@
## master #2838 +/- ##
==========================================
- Coverage 67.47% 67.45% -0.03%
==========================================
Files 341 341
Lines 43355 43362 +7
Branches 5379 5379
==========================================
- Hits 29254 29248 -6
- Misses 13405 13409 +4
- Partials 696 705 +9
Continue to review full report at Codecov.
|
|
Great contribution @feilong! Thanks for getting to the bottom of this! |
effigies
reviewed
Dec 21, 2018
nipype/algorithms/confounds.py
Outdated
| @@ -14,6 +14,7 @@ | |||
| import nibabel as nb | |||
| import numpy as np | |||
| from numpy.polynomial import Legendre | |||
| from scipy import linalg | |||
There was a problem hiding this comment.
Thanks for pointing that out!
effigies
approved these changes
Dec 21, 2018
|
Great! Thanks for making these great softwares. Looking forward to more commits! |
yarikoptic
added a commit
to yarikoptic/nipype
that referenced
this pull request
Feb 4, 2019
1.1.8 (January 28, 2019) * FIX: ANTS LaplacianThickness cmdline opts fixed up (nipy#2846) * FIX: Resolve LinAlgError during SVD (nipy#2838) * ENH: Add interfaces wrapping DIPY worflows (nipy#2830) * ENH: Update BIDSDataGrabber for pybids 0.7 (nipy#2737) * ENH: Add FSL `eddy_quad` interface (nipy#2825) * ENH: Support tckgen -select in MRtrix3 v3+ (nipy#2823) * ENH: Support for BIDS event files (nipy#2845) * ENH: CompositeTransformUtil, new ANTs interface (nipy#2785) * RF: Move pytest and pytest-xdist from general requirement into tests_required (nipy#2850) * DOC: Add S3DataGrabber example (nipy#2849) * DOC: Skip conftest module in API generation (nipy#2852) * DOC: Hyperlink DOIs to preferred resolver (nipy#2833) * MAINT: Install numpy!=1.16.0 from conda in Docker (nipy#2862) * MAINT: Drop pytest-xdist requirement, minimum pytest version (nipy#2856) * MAINT: Disable numpy 1.16.0 for Py2.7 (nipy#2855) * tag '1.1.8': (79 commits) MNT: Add @feilong to .zenodo, update ordering MNT: Update .mailmap MNT: Update .zenodo ordering Accept invitation as Zenodo release co-author (see nipy#2864) MAINT: Update .mailmap BF: allowing bids_event_file as alternate input MNT: Update .zenodo ordering MNT: Version 1.1.8 DOC: 1.1.8 changelog Update nipype/interfaces/dipy/tracks.py Update nipype/interfaces/dipy/reconstruction.py MNT: Install numpy!=1.16.0 from conda in Docker Add FSL auto test remake specs Update nipype/interfaces/io.py Remove return type named tuple Update nipype/info.py STY: Whitespace, line length Remove out_ prefix from EddyQuad outputs Apply minor edits from code review ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The default
lapack_driveruses divide and conquer SVD (gesdd), which occasionally will causeLinAlgErroreven if the data are completely fine. Switchinglapack_drivertogesvdwhen it fails can solve this issue.Summary
Fixes # .
List of changes proposed in this PR (pull-request)
Acknowledgment